Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend native files to store install path information #4743

Merged
merged 6 commits into from
Feb 12, 2019

Conversation

dcbaker
Copy link
Member

@dcbaker dcbaker commented Jan 8, 2019

This series is the start of fixing #4637, though it's not complete. Currently this allows setting builtin path options in the native and cross files. These values will be replaced by command line arguments.

When doing a native (non-cross) build, then the values from the native file will be used, when doing a cross build the values from the cross file will be used. values from the native file will never be used in a cross build, and vice versa. In keeping with the unix tradition, command line arguments overwrite any config file values.

@Ericson2314
Copy link
Member

I agree this should go in the file, but I am hesitant to say that should be the native file. This is not a property of arbitrary machines (build, host, and target); it is as best a property that tends to vary with the host machine.

@Ericson2314
Copy link
Member

Yeah #4637 is a fine idea, but I'd want it to be independent of #3972. If anything, the "flags" file would store what machine files were passed in, like any other command line argument.

@dcbaker
Copy link
Member Author

dcbaker commented Jan 8, 2019

As I mentioned, I'd like to allow these paths to be specified in both the native file (the build machine config files, if you will) as well as the cross file (the host and target machine config files). for cross files though I need to tinker with some assumptions the backend has, namely that there will only be one prefix.

@dcbaker
Copy link
Member Author

dcbaker commented Jan 8, 2019

And I do think it really is a property of the build machine. Take for example libdir (assume x86 and linux),
on Archlinux 64 bit libraries go in /lib, and 32 bit libraries go in /lib32
on Fedora 64 bit libraries go in /lib64 and 32 bit libraries go in /lib32
on debian 64 bit libraries go in /lib/x86_64-linux-gnu and 32 bit go in /lib/i386-linux-gnu

Even if you're doing a "native" build for a different distro you might need a different libdir. I think that the cross file version is probably more interesting from an end user point of view, but not having 3 different files that a user needs to copy to replicate a build is also a feature.

@Ericson2314
Copy link
Member

How is there multiple prefixes with a cross build? There's DESTDIR-like functionality---I'm "installing" it on this machine but I'll actually have to copy the files elsewhere before I use it---but that's different.

In general, the build machine is purely an implementation detail, all interface details should stem from the host machine and target machine. [A prefix in the target machine would be where the newly-build compiler would install things by default, but that doesn't really make much sense.]

@Ericson2314
Copy link
Member

Ericson2314 commented Jan 8, 2019

In short, the host machine is the one on which software runs, the install directory is where the problem runs. Because of the common thread of "program runs", I deem the install dir a host machine property.

@dcbaker
Copy link
Member Author

dcbaker commented Jan 8, 2019

Take this super trivial case:

executable('foo', 'foo.cpp', native : true, install : true)
executable('cross_foo', 'foo.cpp', native : false, install : true)

Currently foo and cross_foo would be installed into the same prefix, even though one is compiled for the build machine and the other for the host machine. This is broken IMHO, which leaves us with two choices:

  1. allow two prefix, one of the build and one for the host
  2. error if trying to install native : true binaries in a cross build

Thinking about it more 2 is probably a better solution, which actually means I can just add my patches to add this to the cross file as well.

@Ericson2314
Copy link
Member

If we want to limit ourselves 2-3 files ("native and cross" or "file per machine") and not more, I'd permit the [path] section in both native and cross files, but simply have the cross file override the native one. This matches how the host_machine defaults to the build_machine if not explicit definition is given.

@Ericson2314
Copy link
Member

Ericson2314 commented Jan 8, 2019

@dcbaker yes 2 is better. I have implemented that in fact in my mammoth PR #4010. Let me find it. I don't see it; I think I was getting confused with the mixed-platforms dependency warning.

@dcbaker
Copy link
Member Author

dcbaker commented Jan 8, 2019

Yeah, here's how I'm leaning:
"native build": cmd line > native file
"cross build": cmd line > cross file

such that in a cross build the paths section from the native file is completely ignored

@Ericson2314
Copy link
Member

Yeah I'm not sure that should be true for all CLI flags, but that the right way for this.

@dcbaker
Copy link
Member Author

dcbaker commented Jan 8, 2019

The unix tradition is that command line > config, and I've been going with that. I think that will surprise people less.

@Ericson2314
Copy link
Member

Oh yes I agree with the direction of the >. I just meant that some options should still come from the native build file in cross builds; it depends on the flag in question.

@dcbaker
Copy link
Member Author

dcbaker commented Jan 8, 2019

Right, what I'm suggesting is in a case like:

[paths]
prefix = '/usr/x86root'
libdir = 'lib32'

and the command line meson builddir --libdir=lib --cross-file=mycross,

that prefix would be /usr/x86root, and the libdir would be lib, command line arguments in unix tradition override config file variables.

@dcbaker dcbaker force-pushed the native-file-extended branch 2 times, most recently from 083b582 to 6398ab6 Compare January 9, 2019 00:24
@dcbaker
Copy link
Member Author

dcbaker commented Jan 14, 2019

ping

@jpakkane
Copy link
Member

Instead of "paths" should this be "options" so you could provide a value for any option, not just paths?

@dcbaker
Copy link
Member Author

dcbaker commented Jan 17, 2019

Well, I'm not sure what the right thing to do is here. In cross files we store some options as properties, like c_args. But you're probably right, it probably makes sense to store these in an [options] section.

@Ericson2314
Copy link
Member

Ericson2314 commented Jan 17, 2019

#4626 makes [properties] and compiler_options play a lot nicer. I'm kind of wary about how free-form options are and wouldn't want that to leak further into config files too.

[paths] and [compiler_options] makes sense to me, with [properties] deprecated. [options] is rather vague; everything in a config file is an option!

@dcbaker
Copy link
Member Author

dcbaker commented Feb 1, 2019

@jpakkane, preferences here, I'd like to get this merged and I'm happy to change the name to whatever you like.

@dcbaker dcbaker force-pushed the native-file-extended branch 4 times, most recently from adba5bf to a3b78b3 Compare February 4, 2019 22:34
@jpakkane
Copy link
Member

jpakkane commented Feb 4, 2019

Let's go with the current [paths]. Even if we had a way to add general command line options, having different types in their own segments includes readability.

# Store paths for native and cross build files. There is no target
# machine information here because nothing is installed for the target
# architecture, just the build and host architectures
self.paths = PerMachine(Directories(), Directories(), None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well by that logic this shouldn't be a PerMachine because nothing is ever installed for the build platform by definition. (It is the host platform, and when build == host, it is because of the host platform, not because of the build platform aliasing it.)

All things target-platform related are dead code in Meson today, btw, but if/when Meson gets used for some compiler, it may be nice to specify per-target paths separately. (Consider multilib GCC or LLVM and their /lib/compiler-config directories.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you wish to keep PerMachine for future target support, use PerMachineDefaultable so that you can default paths.host when no cross file is used, to reflect that the host and build configurations are the same.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but the nice thing about storing it in a PerMachine is that we don't have to special case the cross/native reading machinery. We just load the cross and the native files and then use the right one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, that's not necessarily true. LLVM can build llvm-config for the build system, even when libLLVM.so is compiled for the host system. So it is possible that you could need both.

Copy link
Member

@Ericson2314 Ericson2314 Feb 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hehe, *-config and friends are indeed an exception to my rule. Fair point, but I hope people don't do use this for that and instead switch to pkg-config. xxx-config has caused us using my distro a ton of pain (NixOS/nixpkgs#51176)!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure -config has caused me no end of pain. Unfortunately no one asks me :)

mesonbuild/mesonlib.py Outdated Show resolved Hide resolved
mesonbuild/mesonlib.py Outdated Show resolved Hide resolved
@dcbaker dcbaker force-pushed the native-file-extended branch 3 times, most recently from 060afce to 970f7d7 Compare February 4, 2019 23:54
@dcbaker
Copy link
Member Author

dcbaker commented Feb 7, 2019

@Ericson2314 For the moment I've just used the PerMachineDefaultable as-is. I'm not entirely happy with it, but that is life.

@Ericson2314
Copy link
Member

@dcbaker Sounds good. I'll try some of those factor refactors, say once I'm done with #4010.

@Ericson2314
Copy link
Member

Oh a last thing for this PR, which otherwise looks great. Instead of trying to detect the default and override conditionally, can we always set the options based on the user config before we slurp in CLI flags? See https://github.com/mesonbuild/meson/pull/4626/files#diff-246a2f6d498375c5981f0ca48d99cae2R585 for where I did this with the compiler options

This allows the person running configure (either a developer, user, or
distro maintainer) to keep a configuration of where various kinds of
files should end up.
Just like the previous patch, but for cross files

Fixes mesonbuild#1433
Mypy struggles with the imperative form of Enum declaration, and
upstream doesn't consider it a bug, they recomend using the class form
for enums that are going to be externally exposed.
@dcbaker
Copy link
Member Author

dcbaker commented Feb 11, 2019

@Ericson2314, that was no small ask :) I do think that the result is nicer (no more tracking of default arguments), but it was basically a complete rewrite of the series. Hopefully this version is ready.

@Ericson2314
Copy link
Member

@dcbaker All the more thanks then! I when through it one last time, commit by commit, and it looks good.

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@dcbaker
Copy link
Member Author

dcbaker commented Feb 11, 2019

@jpakkane how do you feel about this?

@jpakkane jpakkane merged commit 82e4cb7 into mesonbuild:master Feb 12, 2019
@dcbaker dcbaker deleted the native-file-extended branch February 12, 2019 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants